Assignment 1 #19

chanderlud · 2025-09-30T04:04:21Z

I worked with @JosephPotapenko on this assignment.

For extra credit, we implemented an AI trivia question generator that uses the OpenAI API to generate more questions once the preset ones run out. In order to use this feature, the OPENAI_API_KEY environmental variable must be set to a valid API key.

…U-CSCD371-2025-Fall into assignment-1-colab

…estion generation

Copilot

Pull Request Overview

This PR adds AI-generated trivia questions to the Princess Bride trivia game using OpenAI's API. After completing the preset questions, the game continues with dynamically generated questions until the user types 'exit'.

Integrated OpenAI API to generate additional Princess Bride trivia questions
Added continuous gameplay loop with exit condition
Fixed integer division bug in percentage calculation

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
Program.cs	Added OpenAI integration, continuous gameplay loop, and fixed percentage calculation
PrincessBrideTrivia.csproj	Added OpenAI package dependency
ProgramTests.cs	Added basic tests for AI question generation functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

Copilot · 2025-09-30T04:05:06Z

PrincessBrideTrivia/PrincessBrideTrivia.Tests/ProgramTests.cs

+    {
+        var apiKey = Environment.GetEnvironmentVariable("OPENAI_API_KEY");
+        Question q = await TriviaGenerator.GeneratePrincessBrideQuestionAsync(apiKey);
+        Assert.HasCount(4, q.Answers);


Assert.HasCount is not a valid MSTest assertion method. Use Assert.AreEqual(4, q.Answers.Length) instead.

Suggested change

Assert.HasCount(4, q.Answers);

Assert.AreEqual(4, q.Answers.Length);

This may have been the standard previously, but with the MSTests that were provided which are more current than the old standards, this is no longer the case, and .HasCount is actually correct.

…lable

PigPig-SDK

Super cool extra credit. Though I cannot say how well the AI semantics play (I cannot be bothered getting an API key). I'm confident the try,catch,async and awaits will be functional.

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

PigPig-SDK · 2025-09-30T19:09:30Z

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

    }

-    public static bool AskQuestion(Question question)
+    public static (bool, bool) AskQuestion(Question question)


Could replace the tuple with [flag] enum for better code clarity.
EX:
[Flags] public enum SorryImBadAtNames: byte { None = 0, AnswerdCorrectly = 1, QuitProgram = 2, };
Then you can use the objectEnum.HasFlag(FLAG); to figure out the response. More info at link:
https://learn.microsoft.com/en-us/dotnet/api/system.enum.hasflag?view=net-9.0

Implementing enum would be a lot messier and a less concise. However, dealing with both the correct answers and quitting the program in one function is a good suggestion and will be implemented.

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

…places for improved user experience

BillMillerCoding

Assignment Details
Pull Request title includes "Assignment 1" in the title ✔
Partner has also made a commit ✔
Pull Request targets Assignment 1 branch ✔
Issue 1: Application no longer crashes ✔
Issue 2: Unit test properly passes ✔
Extra Credit
Come up with a new feature for the application

Issue 3: Add a new feature request implemented ✔
Issue 3: Feature request unit tested ✔
Very impressive in my opinion! The AI feature is super cool and you guys definitely have some know how! great work

BillMillerCoding · 2025-10-02T02:08:05Z

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

-            string answer2 = lines[lineIndex + 2];
-            string answer3 = lines[lineIndex + 3];
+            Question question = new() { Text = lines[lineIndex], Answers = new string[3], CorrectAnswerIndex = lines[lineIndex + 4] };
+            question.Answers[0] = lines[lineIndex + 1];


I like how in this section you condensed the code and made it more streamlined. However, I personally prefer the readability of the previous version. Since I don't see this contradicting any guidelines this is purely a stylistic opinion.

I'd personally chose to stick with the streamlined code because it’s cleaner and easier to maintain. It does the same thing as before but with less repetition, which reduces the chance of errors if future maintenance was to become a concern.

BillMillerCoding · 2025-10-02T02:11:24Z

PrincessBrideTrivia/PrincessBrideTrivia/TriviaGenerator.cs

+            if (string.IsNullOrWhiteSpace(apiKey))
+                throw new ArgumentException("API key is required.", nameof(apiKey));
+
+            if (choices is < 3 or > 6)


Very much grasping for straws here cause your code is so spot on! perhaps you should avoid making the bounds magic numbers and instead having the answer bounds be variables or consts. This would add versatility if the program ever was to be updated with more options

Thanks for the feedback! In this case, I chose to leave the bounds as-is since the program is intentionally simple and unlikely to change. Introducing constants or variables here wouldn’t add much value for this particular use case, but I appreciate the suggestion.

Good discussion you two :) This is definitely what we're looking for here. Respectful discussion and explanation of your reasoning.

Side note: I would tend to agree with William here, though. I think it'd be best practice to set them in local or global fields, for future extensibility, like William said. Always sticking to best practice builds up good habits.

steeley21

Assignment Details

Pull Request title includes "Assignment 1" in the title ✔
Partner has also made a commit ✔
Pull Request targets Assignment 1 branch ✔
Issue 1: Application no longer crashes ✔
Issue 2: Unit test properly passes ✔

Extra Credit

Come up with a new feature for the application
    Issue 3: Add a new feature request implemented ✔
    Issue 3: Feature request unit tested ✔

Cool feature update!

PrincessBrideTrivia/PrincessBrideTrivia.Tests/ProgramTests.cs

…U-CSCD371-2025-Fall into assignment-1-colab

…e Assert.HasCount(...) instead of Assert.AreEqual(..., collection.Length).

…n the same method in order to have more readable code.

… of the program. I changed it to the very beginning so as to be more user friendly. I also fixed the grades to display properly- whether you quit early, or if you finish everything correctly.

github-actions · 2025-10-02T20:58:44Z

Summary


Generated on:	10/02/2025 - 20:58:43
Coverage date:	10/02/2025 - 20:58:40
Parser:	MultiReport (2x Cobertura)
Assemblies:	1
Classes:	3
Files:	3
Line coverage:	22.1% (37 of 167)
Covered lines:	37
Uncovered lines:	130
Coverable lines:	167
Total lines:	249
Branch coverage:	6% (4 of 66)
Covered branches:	4
Total branches:	66
Method coverage:	Feature is only available for sponsors
Tag:	42_18205469505

Coverage

PrincessBrideTrivia - 22.1%

Name	Line	Branch
PrincessBrideTrivia	22.1%	6%
PrincessBrideTrivia.Program	33.6%	13.3%
PrincessBrideTrivia.Question	100%
PrincessBrideTrivia.TriviaGenerator	0%	0%

Joshua-Lester3

Assignment Details

Pull Request title includes "Assignment 1" in the title ✔
Partner has also made a commit ✔
Pull Request targets Assignment 1 branch ✔
Issue 1: Application no longer crashes ✔
Issue 2: Unit test properly passes ✔

Extra Credit

Come up with a new feature for the application
- Issue 3: Add a new feature request implemented ✔
- Issue 3: Feature request unit tested ✔-ish

Honestly, phenomenal extra credit. That's some cool stuff. I tested it out with an OpenAI key and it worked great. The only note I'd make is change how it's tested - but we're not at a point in class that we've gone over that yet. See my comment for some information on how to go about that.

Edit: though I will say, make sure to get all the additional functionality tested and implemented well. I assume a coding assistant was used, solely because of the extra comments with check marks. Definitely encourage that if you're understanding what it's doing and can do it yourself/already did first. But quality testing and implementation comes before extra functionality with lesser quality.

Joshua-Lester3 · 2025-10-06T23:51:01Z

PrincessBrideTrivia/PrincessBrideTrivia.Tests/ProgramTests.cs

+    [TestMethod]
+    public async Task GenerateQuestions_Success()
+    {
+        var apiKey = Environment.GetEnvironmentVariable("OPENAI_API_KEY");
+
+        if (apiKey == null)
+        {
+            return; // test cannot run without the API key
+        }
+
+        Question q = await TriviaGenerator.GeneratePrincessBrideQuestionAsync(apiKey);
+        Assert.IsNotNull(q);
+    }


We probably haven't gotten to the point in class that we could test this easier, but relying on an external API for testing and CI/CD will make the test flaky (sometimes pass, sometimes fail, sometimes won't even test functionality - as it does here, and ultimately passes because no exceptions are thrown).

If you want to look ahead, Moq is a great way to mock implementations to insert "mocked" implementation for testing purposes. It's used mostly on interfaces and abstract classes, so you'd have to rearrange your architecture.

Joshua-Lester3 · 2025-10-06T23:59:16Z

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

+            catch (InvalidOperationException ex)
+            {
+                Console.WriteLine(ex.Message);
+                continue;


I have a feeling if an exception was thrown here, it wouldn't be good to continue on. That'd signal there's something wrong with the OpenAI API or your TriviaGenerator code, and it probably wouldn't fix itself between iterations. Another dangerous thought is, if it was left to run by itself and kept running into this exception, it could run down your OpenAI tokens and increase your budget, since the exception can occur before ever asking for user input.

Joshua-Lester3 · 2025-10-07T00:01:47Z

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

        }
-        Console.WriteLine("You got " + GetPercentCorrect(numberCorrect, questions.Length) + " correct");
+
+        // ✅ If user answered all file questions and we are NOT running AI loop, print score now


Comments (especially, I assume, LLM comments 😉) that don't add clarity to the reader is against guidelines.

Joshua-Lester3 · 2025-10-07T00:03:04Z

PrincessBrideTrivia/PrincessBrideTrivia/Program.cs

    }

-    public static bool AskQuestion(Question question)
+    public static (bool answeredCorrectly, bool quitProgram) AskQuestion(Question question)


I'd recommend against returning Tuples, as I think Mark said on Thursday. Could use an out parameter or maybe restructure logic around determining quitting.

Joshua-Lester3 · 2025-10-07T00:09:07Z

PrincessBrideTrivia/PrincessBrideTrivia/TriviaGenerator.cs

+            if (string.IsNullOrWhiteSpace(apiKey))
+                throw new ArgumentException("API key is required.", nameof(apiKey));
+
+            if (choices is < 3 or > 6)


Good discussion you two :) This is definitely what we're looking for here. Respectful discussion and explanation of your reasoning.

Side note: I would tend to agree with William here, though. I think it'd be best practice to set them in local or global fields, for future extensibility, like William said. Always sticking to best practice builds up good habits.

Joshua-Lester3 · 2025-10-07T00:12:04Z

PrincessBrideTrivia/PrincessBrideTrivia/TriviaGenerator.cs

+            Difficulty: hard.
+        """;
+
+            var completion = await client.CompleteChatAsync(


I'd lean away from using 'var' here. Mark's guidelines advise against it here: AVOID using implicitly typed local variables ( var ) unless the data type of the assigned value is obvious.

Joshua-Lester3 · 2025-10-07T00:12:43Z

PrincessBrideTrivia/PrincessBrideTrivia/TriviaGenerator.cs

+                throw new InvalidOperationException("Model returned empty content.");
+
+            // Strict JSON parse
+            var question = JsonSerializer.Deserialize<Question>(json, new JsonSerializerOptions


Same here, although if the reader has experience with JsonSerializer, they could determine the type. It's not immediately obvious though.

Joshua-Lester3 · 2025-10-07T00:17:50Z

PrincessBrideTrivia/PrincessBrideTrivia/TriviaGenerator.cs

+            if (q is null) throw new InvalidOperationException("Failed to parse the model's JSON.");
+            if (string.IsNullOrWhiteSpace(q.Text)) throw new InvalidOperationException("Question text missing.");
+            if (q.Answers is null || q.Answers.Length != expectedChoices)
+                throw new InvalidOperationException($"Expected {expectedChoices} answers, got {q?.Answers?.Length ?? 0}.");
+
+            if (q.Answers.Any(string.IsNullOrWhiteSpace))
+                throw new InvalidOperationException("One or more answers are empty.");
+
+            if (!int.TryParse(q.CorrectAnswerIndex, out var idx))
+                throw new InvalidOperationException("CorrectAnswerIndex must be a numeric string.");
+
+            if (idx < 0 || idx >= q.Answers.Length)
+                throw new InvalidOperationException("CorrectAnswerIndex out of range.");


Suggested change

if (q is null) throw new InvalidOperationException("Failed to parse the model's JSON.");

if (string.IsNullOrWhiteSpace(q.Text)) throw new InvalidOperationException("Question text missing.");

if (q.Answers is null || q.Answers.Length != expectedChoices)

throw new InvalidOperationException($"Expected {expectedChoices} answers, got {q?.Answers?.Length ?? 0}.");

if (q.Answers.Any(string.IsNullOrWhiteSpace))

throw new InvalidOperationException("One or more answers are empty.");

if (!int.TryParse(q.CorrectAnswerIndex, out var idx))

throw new InvalidOperationException("CorrectAnswerIndex must be a numeric string.");

if (idx < 0 || idx >= q.Answers.Length)

throw new InvalidOperationException("CorrectAnswerIndex out of range.");

if (q is null)

{

throw new InvalidOperationException("Failed to parse the model's JSON.");

}

if (string.IsNullOrWhiteSpace(q.Text))

{

throw new InvalidOperationException("Question text missing.");

}

if (q.Answers is null || q.Answers.Length != expectedChoices)

{

throw new InvalidOperationException($"Expected {expectedChoices} answers, got {q?.Answers?.Length ?? 0}.");

}

if (q.Answers.Any(string.IsNullOrWhiteSpace))

{

throw new InvalidOperationException("One or more answers are empty.");

}

if (!int.TryParse(q.CorrectAnswerIndex, out var idx))

{

throw new InvalidOperationException("CorrectAnswerIndex must be a numeric string.");

}

if (idx < 0 || idx >= q.Answers.Length)

{

throw new InvalidOperationException("CorrectAnswerIndex out of range.");

}

Might just be a personal thing, but it's not immediately easy to read through this block. Even though certain statements and blocks can be abbreviated, I'd stick towards making them clearer. Although it's a little more typing, I'd do the suggestion above. Or even having the first two if statements not be one line total.

Here's what Mark's guidelines say about it: AVOID omitting braces, except for the simplest of single-line if statements.

Joshua-Lester3 · 2025-10-07T00:19:12Z

PrincessBrideTrivia/PrincessBrideTrivia.sln

 # Visual Studio Version 17
-VisualStudioVersion = 16.0.29215.179
+VisualStudioVersion = 17.14.36518.9 d17.14
 MinimumVisualStudioVersion = 10.0.40219.1


I don't think this should be here. Be sure to review all changes before staging/committing.

Joshua-Lester3 · 2025-10-07T00:23:14Z

@chanderlud @JosephPotapenko make sure to respond to and resolve all comments (even Copilot's review).

chanderlud and others added 7 commits September 29, 2025 20:19

casts numbers to float for correct percent calculation

a59ac2f

Adding Questions into the array

6405f02

Merge branch 'assignment-1-colab' of https://github.com/chanderlud/EW…

0903549

…U-CSCD371-2025-Fall into assignment-1-colab

WIP AI generated trivia questions

0024d32

Merge branch 'assignment-1-colab' of https://github.com/chanderlud/EW…

8d49d27

…U-CSCD371-2025-Fall into assignment-1-colab

finished AI generated trivia questions & adds basic unit tests for qu…

3c4ff18

…estion generation

Updated comments

b728c81

Copilot AI review requested due to automatic review settings September 30, 2025 04:04

Copilot AI reviewed Sep 30, 2025

View reviewed changes

chanderlud added 2 commits September 29, 2025 21:06

disables question generation tests when an OpenAI API key is not avai…

1ec8dca

…lable

improved error handling and flow

deb917e

PigPig-SDK approved these changes Sep 30, 2025

View reviewed changes

chanderlud added 4 commits September 30, 2025 14:01

refactors TriviaGenerator class to its own file

ac8b0fe

simplifies LoadQuestions by removing temporary variables

a8d2e1b

refactors to string interpolation, rounds the percent to two decimal …

3c0106b

…places for improved user experience

resolves several warnings

1a8065d

BillMillerCoding reviewed Oct 2, 2025

View reviewed changes

steeley21 reviewed Oct 2, 2025

View reviewed changes

PrincessBrideTrivia/PrincessBrideTrivia.Tests/ProgramTests.cs Outdated Show resolved Hide resolved

JosephPotapenko and others added 6 commits October 1, 2025 21:34

Fixing the .HasCount to .AreEqual in order to follow proper test syntax.

6a134af

Merge branch 'assignment-1-colab' of https://github.com/chanderlud/EW…

3148200

…U-CSCD371-2025-Fall into assignment-1-colab

Reverting to .HasCount because apparently the MSTest analyzers requir…

c40d23b

…e Assert.HasCount(...) instead of Assert.AreEqual(..., collection.Length).

Making the checking correct answer and the quitting functionalities i…

75e8527

…n the same method in order to have more readable code.

Previously, the "type quit to exit message" displayed at the very end…

8dffbfa

… of the program. I changed it to the very beginning so as to be more user friendly. I also fixed the grades to display properly- whether you quit early, or if you finish everything correctly.

renames tests as suggested

cdb3d52

Joshua-Lester3 reviewed Oct 7, 2025

View reviewed changes

	Assert.HasCount(4, q.Answers);
	Assert.AreEqual(4, q.Answers.Length);

Assignment 1 #19

Are you sure you want to change the base?

Assignment 1 #19

Conversation

chanderlud commented Sep 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PigPig-SDK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BillMillerCoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillMillerCoding Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steeley21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 2, 2025

Summary

Coverage

Uh oh!

Joshua-Lester3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Assignment Details

Extra Credit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joshua-Lester3 commented Oct 7, 2025

Uh oh!

Uh oh!

BillMillerCoding Oct 2, 2025 •

edited

Loading

Joshua-Lester3 left a comment •

edited

Loading